Skip to content

Double pinyin query #2427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 39 commits into
base: dev
Choose a base branch
from
Draft

Double pinyin query #2427

wants to merge 39 commits into from

Conversation

VictoriousRaptor
Copy link
Contributor

No description provided.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@VictoriousRaptor VictoriousRaptor marked this pull request as draft November 19, 2023 03:48
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
taooceros
taooceros reviewed Dec 28, 2023
View reviewed changes
Flow.Launcher.Infrastructure/DoublePinAlphabet.cs Outdated
{"ian", "m"}
});

private static string ToDoublePin(string fullPinyin)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to use ReadOnlySpan as we have quite a few slicing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@taooceros
Copy link
Member

@VictoriousRaptor what's the status of this pr?

@VictoriousRaptor
Copy link
Contributor Author

@VictoriousRaptor what's the status of this pr?

POC. I'm using it daily and so far so good. Since there're many double pinyin mappings we should make it user configurable. Is it acceptable to allow users to edit a json file that defines the mapping?

@taooceros
Copy link
Member

well I can't think anything better...maybe I do want this to be plugin-able, but can't think of a good way to design the api.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Jack251970 Jack251970 requested a review from Copilot April 10, 2025 01:58
Copilot

This comment was marked as outdated.

Sign in to view

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@VictoriousRaptor VictoriousRaptor requested a review from Copilot June 14, 2025 07:47
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
Copilot
Copilot AI reviewed Jun 14, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for a double pinyin query by introducing a new double_pinyin.json resource and integrating this functionality into the settings and pinyin translation mechanisms.

  • Adds a new JSON resource for double pinyin mappings.
  • Updates the project file to include the new resource.
  • Extends Settings, TranslationMapping, StringMatcher, and PinyinAlphabet to support double pinyin translation.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Flow.Launcher/Resources/double_pinyin.json New resource file containing pinyin mappings.
Flow.Launcher/Flow.Launcher.csproj Updated to copy the double_pinyin.json resource to output.
Flow.Launcher.Infrastructure/UserSettings/Settings.cs Added new settings (UseDoublePinyin and DoublePinyinSchema).
Flow.Launcher.Infrastructure/TranslationMapping.cs No functional changes; existing mapping logic is maintained.
Flow.Launcher.Infrastructure/StringMatcher.cs Adjustments to pinyin translation checks and method modifiers.
Flow.Launcher.Infrastructure/PinyinAlphabet.cs Major modifications to load double pinyin data and update pinyin translation logic.
Flow.Launcher.Infrastructure/IAlphabet.cs Minor interface update reflecting translation API changes.

Flow.Launcher.Infrastructure/PinyinAlphabet.cs

bool pre = false;
var previousIsChinese = false;
Copy link
Preview

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the BuildCacheFromContent loop, the flag 'previousIsChinese' is never updated after processing a Chinese character, which prevents the intended spacing logic from triggering. Consider setting 'previousIsChinese = true' after appending a Chinese pinyin translation (and resetting it appropriately in the non-Chinese branch).

Copilot uses AI. Check for mistakes.

@github-actions GitHub Actions
Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors and Warnings Count
❌ forbidden-pattern 22
⚠️ non-alpha-in-dictionary 13
⚠️ single-line-file 1

See ❌ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taooceros taooceros taooceros left review comments

@Jack251970 Jack251970 Jack251970 left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@VictoriousRaptor VictoriousRaptor

Labels
20 min review Proof of Concept
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VictoriousRaptor @taooceros @Jack251970 @jjw24